Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EL6/7 and CentOS 6/7 support for st2 and st2repos #66

Merged
merged 45 commits into from
Jan 21, 2017

Conversation

Mierdin
Copy link
Member

@Mierdin Mierdin commented Jan 4, 2017

This patch adds support for the following operating systems:

  • CentOS 6
  • CentOS 7
  • RHEL 6
  • RHEL 7

I have also verified that an Ubuntu Trusty install still works after these changes.

The following roles have been created:

  • epel

The following roles have been modified (either to support epel installation, or to work on EL/CentOS in general):

  • st2
  • st2repos
  • mongodb
  • st2mistral

@Mierdin Mierdin changed the title RHEL Support for st2 and st2repos roles [WIP] RHEL Support for st2 and st2repos roles Jan 4, 2017
@Mierdin Mierdin changed the title [WIP] RHEL Support for st2 and st2repos roles [WIP] CentOS 7 Support for st2 and st2repos roles Jan 4, 2017
@Mierdin Mierdin changed the title [WIP] CentOS 7 Support for st2 and st2repos roles CentOS 7 Support for st2 and st2repos roles Jan 4, 2017
Copy link
Contributor

@bigmstone bigmstone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided some comments below.

Can you check that it 100% works on a real RHEL not just CentOS? You can spin up AWS instance to test it.

Apart of that, since you're adding support for more OSes, can you sync up meta file: https://github.com/Mierdin/ansible-st2/blob/8f1e76b94b974379ead556b67f8fec2ec170338b/roles/st2/meta/main.yml#L8 as well?

notify:
- restart st2
- include: debian.yml
when: ansible_os_family == 'Debian'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do 1 include without when condition.

See @humblearner arppoach

include: mongodb_{{ ansible_pkg_mgr }}.yaml
with 2 files mongodb_apt.yml and mongodb_yum.yaml which looks nicer and cleaner.

It would be better if we follow one standard in all roles for consistency.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 6a4467f

yum:
name: st2
state: latest
disable_gpg_check: yes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we have disable_gpg_check: yes for stable and nothing for unstable in next block? Any specific reason?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, just missed it. Added in 5a1f2e4


- name: Add keys to keyring
become: yes
apt_key:
Copy link
Member

@arm4b arm4b Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be more solid if we check id (identifier of key) here as well.
It wasn't there before, but it's good time to add it.

See example with Mongo:

id: 42F3E95A2C4F08279C4960ADD68FA50FEA312927

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where would I go about finding that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, added in f6ac495

name: stackstorm
description: Stackstorm Repo
file: st2
baseurl: https://packagecloud.io/StackStorm/stable/el/$releasever/$basearch
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing {{ st2_pkg_repo }} variable here which is responsible for repository we install (stable/unstable etc).

See: https://github.com/StackStorm/ansible-st2#variables and existing rule in deb: https://github.com/StackStorm/ansible-st2/pull/66/files#diff-37045cfaa2d3524070e708be840837adR21

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in c40a809

yum_repository:
name: stackstorm
description: Stackstorm Repo
file: st2
Copy link
Member

@arm4b arm4b Jan 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is yum repo file I got for CentOS when adding st2 packagecloud repo:

[vagrant@centos7 ~]$ cat /etc/yum.repos.d/StackStorm_stable.repo
[StackStorm_stable]
name=StackStorm_stable
baseurl=https://packagecloud.io/StackStorm/stable/el/7/$basearch
repo_gpgcheck=1
gpgcheck=0
enabled=1
gpgkey=https://packagecloud.io/StackStorm/stable/gpgkey
sslverify=1
sslcacert=/etc/pki/tls/certs/ca-bundle.crt
metadata_expire=300

A lot of security checks here by PackageCloud. I think we should include more yum_repository options where it's possible and looks easy.

Additionally, it would be nice to follow the same naming for the file and repo.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So would you prefer I undo 8f1e76b? I was using the script to automatically generate this configuration previously

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm for hardcoding safe values we can use easily via yum_repository, without auto-generating them.

@arm4b
Copy link
Member

arm4b commented Jan 5, 2017

@Mierdin Do you have in plans adding EL6 support in this PR or it will be another one?
At least @humblearner adding EL6 as well in his mongodb PR: #65

Additionally, after reviewing both PRs I believe we should add these platforms in CI first for faster feedback loop, testing and better quality (see #69 and #68).

@Mierdin
Copy link
Member Author

Mierdin commented Jan 5, 2017

@armab I don't see anything that would prevent CentOS6 from running, I just haven't tested it. I'll test and update the title accordingly, this should allow both.

Also I am on board for modifying CI before merging this.

@arm4b arm4b added this to the v0.6.0 milestone Jan 11, 2017
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So yeah, as in any other PR the requests are similar:

  • all platforms we support end-to-end testing
    • Xenial (aleady covered by TravisCI)
    • Trusty (already covered by TravisCI)
    • CentOS6
    • CentOS7
    • RHEL6 (via AWS)
    • RHEL7 (via AWS)
  • Ansible Idempotence
    Eg. Ansible-playbook re-run should end with the following results: changed=0.*failed=0

- name: Install epel-release repo (RedHat)
become: yes
yum:
name: epel-release
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this work on real RHEL6?
You can test in AWS https://aws.amazon.com/marketplace/pp/B00CFQWLS6

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I will include both EL6 and EL7 in my testing.

Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So here we install epel-release and still including epel role?
Needs improvement to avoid repeating similar tasks.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e9ed597

when: st2_version != "stable"
notify:
- restart st2
- include: "{{ ansible_os_family }}.yml"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it only about installing package, you can avoid splitting into {{ ansible_os_family }}.yml here, just use package module. http://docs.ansible.com/ansible/package_module.html

See solution in st2mistral by @humblearner

- name: Install latest st2mistral
become: yes
package:
name: st2mistral
state: latest
when: mistral_version == "stable"
tags: st2mistral
- name: Install latest st2mistral
become: yes
package:
name: st2mistral={{ mistral_version }}
state: present
when: mistral_version != "stable"
tags: st2mistral
which is pretty elegant approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 3c1fd14

apt:
name: "{{ item }}"
state: present
update_cache: yes
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will break Ansible Idempotence as @bigmstone discovered before, since it will apt-get upgrade every time.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 025ed66

yum_repository:
name: stackstorm
description: Stackstorm Repo
file: st2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm for hardcoding safe values we can use easily via yum_repository, without auto-generating them.

@arm4b arm4b changed the title CentOS 7 Support for st2 and st2repos roles EL6/7 Support for st2 and st2repos roles Jan 16, 2017
@Mierdin Mierdin changed the title EL6/7 Support for st2 and st2repos roles EL6/7 and CentOS 6/7 support for multiple roles Jan 18, 2017
@Mierdin
Copy link
Member Author

Mierdin commented Jan 20, 2017

@armab @mstone Okay for reals this time, I have tested on EL6 and EL7, and I also re-ran this on CentOS 6/7 and Ubuntu to make sure those still work.

Only outstanding issue is to make sure that I did the right thing here: f5bb4b5

I had to do this to get rid of the error: sudo: sorry, you must have a tty to run sudo

This was originally implemented in 11129e9 but everything seems to run just fine without it.

become: yes
package:
name: ca-certificates
state: latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[ANSIBLE0010] Package installs should not use latest

The ansible-lint warning is valid. Use state: installed where possible.
Installing latest package version on every playbook re-run is generally a bad practice (especially when ansible works in ansible-pull mode).

For st2 we use latest intentionally as option so user can specify st2_version=latest to have latest StackStorm.
Probably we should change default behavior there someday later since it could be dangerous for default installs.

For everything else the safe way is to upgrade software on the server by running specific playbook which focuses on OS package upgrades (or patches) to have full control and react accordingly if something went wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear, for the st2 role, you're saying I should provide a var that controls the parameter passed there, defaulting to latest?

Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just change to state: installed here, as ansible-lint suggests.

[ANSIBLE0010] Package installs should not use latest

Sorry for my previous TL;DR comment ^^ 😄 Just wanted to give an explanation.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the problem I had with this specific line (not other places where I made this linting error) was that unless I updated ca-certificates, I'd get this error when st2mistral runs:

Failure talking to yum: Cannot retrieve repository metadata (repomd.xml) for repository: StackStorm_stable. Please verify its path and try again

I just checked, and setting this to present instead of latest causes the error to come back up, as the package is already installed, so it doesn't do anything.

Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense for an exception 👍

This looks like old EL6 version according to http://unix.stackexchange.com/questions/21310/yum-cannot-retrieve-repository-centos-6

If that happens on EL6 only, can you add conditional check to run this task only on EL6?
And we can add skip_ansible_lint tag for this block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may have a better fix for this, give me a few minutes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@armab Okay, this seems to work: 9c52c7b Thoughts?

Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mierdin It's not clear how 9c52c7b fixes the issue with outdated ca-certificates.

According to http://stackoverflow.com/questions/26734777/yum-error-cannot-retrieve-metalink-for-repository-epel-please-verify-its-path and http://unix.stackexchange.com/questions/21310/yum-cannot-retrieve-repository-centos-6 2 possible fixes for outdated EL6 are:

  • use http instead of https for package repo
  • update ca-certificates package

How yum -y repolist fixes that? I'm probably don't have enough context here.
Additionally, can you reproduce this issue? As I understood it happens in some outdated EL 6.5 distro.

I'd say, - we don't support some outdated RHEL6.5 which was released in 2013.

So after all, - I'm +1 to remove this block completely :) WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, per our slack conversation, I reverted back to the old approach and am now updating ca-certificates using the yum module, in d8714bb

apt:
package:
name: ftp://fr2.rpmfind.net/linux/centos/6/os/x86_64/Packages/libffi-devel-3.0.5-3.2.el6.x86_64.rpm
state: latest
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 7fa3a70

name: ftp://fr2.rpmfind.net/linux/centos/6/os/x86_64/Packages/libffi-devel-3.0.5-3.2.el6.x86_64.rpm
state: latest
when: ansible_os_family == "RedHat" and ansible_distribution_major_version == "6"
ignore_errors: yes # Ignore error when already installed
Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a hack, - generally a sign of improvement. There should be a better way.

Additionally, I'm not sure if this block is idempotent.
Please double check that we don't download ftp://fr2.rpmfind.net/linux/centos/6/os/x86_64/Packages/libffi-devel-3.0.5-3.2.el6.x86_64.rpm on every playbook re-run.

Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per https://docs.stackstorm.com/install/rhel6.html#system-requirements did you check if it's possible to enable server-optional repository for RHEL6? That would be more elegant and preferable.
So this package dependency will be installed automatically by st2.

See libffi-devel is in st2 package requirements: https://packagecloud.io/StackStorm/stable/packages/el/6/st2-2.1.1-1.x86_64.rpm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried that before, got this:

[root@ip-10-0-4-91 ~]# subscription-manager repos --enable rhel-6-server-optional-rpms
Error: rhel-6-server-optional-rpms is not a valid repository ID. Use --list option to see valid repositories.
[root@ip-10-0-4-91 ~]# subscription-manager repos --list
This system has no repositories available through subscriptions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, this task does run idempotently, now that I've changed to state: latest:

TASK [st2 : Install libffi-devel on EL6] ***************************************
ok: [10.0.4.91]

This should also make the ignore_errors statement pointless, so I have removed it in 100ca71. So as long as you don't prefer the server-optional approach (and know what I'm doing wrong that is preventing me from installing libffi-devel that way) then this should work better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1
Thanks for explanation!

@@ -16,3 +16,6 @@ galaxy_info:
- 7
categories:
- system
dependencies:
- role: epel
become: yes
Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using become: yes when including anything external is usually a bad sign.

The good practice is to become: yes for every task in a role where it's 100% needed.
Eg. we do privilege escalation selectively per task, not per role.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ack, fixed in 9988842

@@ -16,3 +16,6 @@ galaxy_info:
- 7
categories:
- system
dependencies:
- role: epel
Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have conditional check:
Include epel only when ansible_os_family == "RedHat".

Even despite there is a OS check in epel role, having condition here as well will look a bit better by logic.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 9988842

- st2mistral
- st2
Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep the original order.

We install st2 first (more significant) and then st2mistral.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 56ed120

when: ansible_os_family == "RedHat" and ansible_distribution_major_version == "6"
ignore_errors: yes # Ignore error when already installed

- name: Install latest st2 package (stable)
Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(stable) is not exactly what we install here.

Package repository (if you meant that) is controlled via st2_pkg_repo, see: https://github.com/StackStorm/ansible-st2#variables

So (stable) note could be removed since it's irrelevant for this particular block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, my intention with changing this was for easier debugging. Since both tasks had the same name, it was hard to tell from the output which one actually ran.

This should work better: 3c3f272

name: st2
state: latest
when: st2_version == "latest"
notify:
- restart st2
tags: skip_ansible_lint

- name: Install latest st2 package
- name: Install latest st2 package (not stable)
Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as https://github.com/StackStorm/ansible-st2/pull/66/files#r97132662

(not stable) note could be removed.

We can write name: Install pinned st2 package version or similar explanation instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 3c3f272

# Fixes "Failure talking to yum: Cannot retrieve repository metadata (repomd.xml) for repository: StackStorm_stable. Please verify its path and try again" when installing st2
- name: Update ca-certificates package
become: yes
package:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since this block is part of redhat.yml, I think it's more expected to use native yum module for installation, instead of cross-platform package module.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in d8714bb

sslverify: yes

# Fixes "Failure talking to yum: Cannot retrieve repository metadata (repomd.xml) for repository: StackStorm_stable. Please verify its path and try again" when installing st2
- name: Update ca-certificates package
Copy link
Member

@arm4b arm4b Jan 20, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this fixes specific error for PackageCloud repository, maybe it's worth to move it before the
name: Add ST2 Repo block?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reordered in d8714bb

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Really a lot of work and end-to-end testing here, Thank You @Mierdin for taking it so seriously!

Besides of minor changes remove name: epel-release and reverting ca-certificates package we're good to merge.

@Mierdin
Copy link
Member Author

Mierdin commented Jan 20, 2017

@armab ack your latest. Everything seems to work on EL6, and I addressed your two outstanding comments, so if those pass, this should be gtg. Will test on EL7 and CentOS6/7 and ubuntu, then merge latest master, and will let you know via slack when ready

Matt Oswalt added 2 commits January 20, 2017 16:42
The old FTP location was giving me some bad connection issues,
this seems more stable
@Mierdin Mierdin merged commit 9dd652e into StackStorm:master Jan 21, 2017
@Mierdin Mierdin deleted the centos-st2-role branch January 21, 2017 09:01
@arm4b arm4b added refactor and removed WIP labels Jan 25, 2017
@arm4b arm4b changed the title EL6/7 and CentOS 6/7 support for multiple roles EL6/7 and CentOS 6/7 support for st2 and st2repos Jan 25, 2017
@blag blag added the OS support Support/issues/PRs on a specific OS label Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature OS support Support/issues/PRs on a specific OS refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants